Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great improvement for cross-platform compatibility, replacing shell-specific commands with Node.js-based alternatives. The changes are clear and well-described. My review includes a critical point about the required Node.js version introduced by a new dependency, which could impact compatibility. I've also added a suggestion regarding automated testing for these build changes to align with the repository's style guide and ensure long-term stability.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…ari-silvers/A2UI into fix/windows-supported-frontend
hari-silvers
requested review from
ava-cassiopeia,
ditman,
dmandar,
gspencergoog,
jacobsimionato,
paullewis and
zeroasterisk
as code owners
|
It looks like there are extra directories that you didn't mean to commit in the |
ava-cassiopeia
left a comment
ava-cassiopeia
left a comment
There was a problem hiding this comment.
It looks like there are extra directories that you didn't mean to commit in the
renderers/lit/src/v0_8andrenderers/lit/src/0_8directories.
Please fix, and I can re-review after that.
…ari-silvers/A2UI into fix/windows-supported-frontend
hari-silvers
left a comment
hari-silvers
left a comment
There was a problem hiding this comment.
I'll clean up the duplicate directory and force-push an updated branch shortly.
Thanks for catching that.
Updated dependencies and versions in package-lock.json.
There was a problem hiding this comment.
Can this just be added as another job to an existing workflow file, like the web build?
| # --- Build web_core first --- | ||
| - name: Install web_core deps | ||
| working-directory: renderers/web_core | ||
| run: npm install |
There was a problem hiding this comment.
You'll want to prefer npm ci over install where possible: https://docs.npmjs.com/cli/v8/commands/npm-ci
This command is similar to npm install, except it's meant to be used in automated environments such as test platforms, continuous integration, and deployment [...]
| run: npm install | |
| run: npm ci |
| # --- Then build lit --- | ||
| - name: Install lit deps | ||
| working-directory: renderers/lit | ||
| run: npm install |
There was a problem hiding this comment.
Same here
| run: npm install | |
| run: npm ci |
| "wireit": "^0.15.0-pre.2" | ||
| }, | ||
| "dependencies": { | ||
| "@a2ui/web_core": "file:../web_core", |
There was a problem hiding this comment.
This doesn't look right - we want to rely on the published version, with npm link for local dev.
3 participants
Description
Before:
After :
Pre-launch Checklist